feat(trace-utils)!: search all spans to populate tracer payload fields#1954
feat(trace-utils)!: search all spans to populate tracer payload fields#1954duncanpharvey wants to merge 5 commits intomainfrom
Conversation
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
duncanpharvey
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1954 +/- ##
==========================================
+ Coverage 71.80% 72.70% +0.90%
==========================================
Files 434 448 +14
Lines 70607 73679 +3072
==========================================
+ Hits 50697 53569 +2872
- Misses 19910 20110 +200
🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 963dadf | Docs | Datadog PR Page | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
duncanpharvey
requested review from
a team,
Lewis-E and
lucaspimentel
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd5c1b9464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(v) = root.meta.get(field) { | ||
| return Some(v.clone()); |
There was a problem hiding this comment.
When a root span carries the requested key with an empty string (for example version: "") and a child span in the same trace has the non-empty value, this returns immediately with the empty value and never checks the remaining spans. The caller then leaves app_version/env/etc. empty for single-trace payloads, which misses the stated goal of finding a non-empty value when the root span has an empty tag.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| assert!(!span.meta.contains_key("aas.site.type")); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
So I was look at pb:Span, and it looks like there's a bunch of fuzzing used to test that code. Do you think that a bolero check!() type test might be useful here? How well formed are trace payloads, generally?
There was a problem hiding this comment.
collect_pb_trace_chunks, where search_trace_for_field is called, is certainly complex enough to warrant a bolero test in general for everything that it's doing. But for what we want to test as part of this PR in search_trace_for_field - "best effort, first non-empty value wins from any span regardless of structural validity". It just means the bolero test can only assert "if a value is returned, it came from some span in the trace", which is already guaranteed from the other unit tests.
| // from an earlier trace. | ||
| let root = &trace[root_span_index]; | ||
| if tracer_payload_tags.env.is_empty() { | ||
| if let Some(v) = search_trace_for_field(root, trace, "env") { |
There was a problem hiding this comment.
So this trace gets normalized up here right: https://github.com/DataDog/libdatadog/pull/1954/changes#diff-e35e80d6b64d2cc411fc38dad84cf1cf35bd47501f9ef20761c1d20c1b5abd26R631
But it looks like that normalization can have errors, which are logged, but don't break the loop. This is sort of why I'm wondering about fuzzing/trace payloads. Can we get incorrect children passing on incorrect tags?
There was a problem hiding this comment.
Between these normalize functions, only the env field is normalized for the fields relevant to the tracer payload change (env, version, _dd.hostname, runtime-id).
There is a scenario where normalization fails on a span and later spans are not normalized, resulting in potentially non-normalized env values being set on the tracer payload. I think the safest way to handle this scenario would be to apply normalization in collect_pb_trace_chunks rather than modifying how normalize_trace short circuits on an error when iterating over spans.
if let Some(mut v) = search_trace_for_field(root, trace, "env") {
normalize_utils::normalize_tag(&mut v);
tracer_payload_tags.env = v;
}
3 participants
What does this PR do?
When extracting span tags to set in the payload, iterate through each trace to find a non-empty value for the tag. Search root spans on each trace first then additional spans if the root span has an empty value for the specified tag.
Motivation
If a trace payload contains multiple trace chunks, and one of those chunks does not contain a trace that has a value for a given tag (like
version), then the top level field set in the tracer payload could be empty even when other traces could have non-empty values for the same tag.This situation arose with the .NET tracer sending a
command_executionspan with no version in the same payload as anazure_functions.invokespan with a version. As a result, the trace metric generated from this payload,trace.azure_functions.invoke.hits, did not have a version tag set.https://datadoghq.atlassian.net/browse/SVLS-9006
Additional Notes
RootSpanTagstoTracerPayloadTagsto better reflect the source of the populated fields.How to test the change?
Unit tests: